The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: youknowriad <[email protected]>
Co-authored-by: mtias <[email protected]>
Co-authored-by: talldan <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: noisysocks <[email protected]>
Co-authored-by: ellatrix <[email protected]>
Co-authored-by: gziolo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Size Change: +4.28 kB (+0.24%)

Total Size: 1.76 MB

_{compressed-size-action}

When did we start doing this? The last thing I remember was that we wait to set attributes until the image is uploaded until we set the URL attribute, and the blob was just a local state. That's how the undo trap was fixed too. I wonder what changed where we put blob URLs in the attributes?

Super dumb idea:

What if we saved images when the post is saved, and replace URLs server side as images are uploaded and URLs resolved? This would also fix any sort of undo issues.

I wonder what changed where we put blob URLs in the attributes?

The file type block transformations. Currently, it can't be avoided there.

When did we start doing this? The last thing I remember was that we wait to set attributes until the image is uploaded until we set the URL attribute, and the blob was just a local state. That's how the undo trap was fixed too. I wonder what changed where we put blob URLs in the attributes?

To be honest, that's very old, almost from the start (when we implemented drag and drop files)

What if we saved images when the post is saved, and replace URLs server side as images are uploaded and URLs resolved? This would also fix any sort of undo issues.

Not dumb at all but I think it's way too complex to implement. It would be good if we had a way to "create entities" with temporary ids or things like that and refer to them in other places of our data (things like categories, tags, images, even patterns, ...) but it's a huge and very unclear undertaking.

Sometimes we want an attribute that is not serialised to HTML and is not copied when you duplicate a block. For example: blob, widgetId. Other times we want an attribute that is serialised to HTML but is not copied when you duplicate a block. For example: productId.

This PR handles the former but not the latter. Do you think we should, in addition to __experimentalRole = 'local' to handle the former, add another __experimentalRole to handle the latter? Then we can close #29693.

I'm not sure, why would you not want to duplicate a product block with a productID ? I'm wondering if there's something we're missing on the UX side here and that duplication is actually fine here. Do you have more use-cases in mind.

Overall though, you I think it should be fine to have a shouldDuplicate = false or something but to be honest, I have a lot of doubt here. Because can be created in different ways anyway (outside duplication), like programmatically and in that case, it's not possible to handle duplication, so this leave me to think that maybe a better UX here is to actually show something in the block itself that it's a duplicate block, like we do for blocks that should only be used once.

I'm actually even hesitant to prevent these local attributes from being duplicated. Sure, in our current case it will result in two image blocks with the same images but uploaded twice but isn't that better than an empty image and an uploaded image from a user's perspective.

For the widgetId and as implemented today, I think it can be seen as a "local" attribute but ultimately, I also wonder if our widgets editor is the right UI for these and I would love if in the future (I know it's not priority) we merge it with template parts as widget areas are template parts are actually the exact same thing, so I can see the site editor's approach to editing template parts used for widget editor too (we could even use the @wordpress/editor package in that case). Which makes me think that we shouldn't be optimizing for that use-case for now.

I'm not sure, why would you not want to duplicate a product block with a productID ? I'm wondering if there's something we're missing on the UX side here and that duplication is actually fine here. Do you have more use-cases in mind.

From #29693:

I'm working on the Jetpack Pay with PayPal block. It contains a productId attribute which uniquely identifies the actual product record referenced by the block. When a new block is inserted this attribute will be undefined; we hit the API to create a new product, and store the returned id in this attribute.

When the block is duplicated, I'd like to create a new product record with identical fields (title/price/etc). But because the productId is cloned along with all the other attributes, both blocks will point to the same resource. It's possible to keep track of the block's unique clientId and reset the product when this value changes — but while this detects block duplication, it also changes every time the post loads, and I only want to reset the product on duplication.

OK, but if we don't care about the productId use case or the widgetId use case then why not go with an approach that doesn't require a new API?

Either something like 37d6855 which stores the blob in a module variable.

Or set the blob attribute when calling createBlock() but don't add blob to block.json. The block editor currently ignores attributes that aren't in block.json when serialising a block. In other words this PR should work fine without the changes to packages/block-library/image/block.json and packages/blocks.

Here's yet another duplication-related issue, #55823.

@noisysocks I'm not disregarding the duplication issue but I think it's less important and unclear to me still. It feels like a separate concern to me at this point than local attributes.

For the solution you propose, it doesn't work, because we want to be able to create blocks with these temporary attributes from outside the block itself (block transforms...) and createBlock function calls sanitize the attributes and remove the unnecessary ones. I think there's value in being explicit about the things that you can pass when creating a block.

I updated the title to use the local keyword to reflect the implementation better.

As a follow-up, it would finally rename the field __experimentalRole to role and document it so folks can use it confidently in 3rd party projects. It seems like a step in the right direction to help resolve several similar issues outlined by @noisysocks in #49466 and highlighted in this PR.

One potential solution that has been considered is to block "saving" when an upload is in progress. I think that's not ideal and doesn't address the root cause where anyone can actually trigger the saving programmatically.

I still feel the UI should reflect the incomplete state of uploading media. If the media shows a spinner and the image is being uploaded, users should be prevented from publishing a post or from updating a published post.

If they do so, the front end of the post will reflect a "broken" state where media is only partially uploaded. Even if revisiting the editor will automatically complete the upload, we should provide a better user experience by not letting users inadvertently publish broken content.

See #39223

@adamsilverstein yes, I didn't close that issue because I also think there's value in letting the user know that there are things in progress. I think both things are necessary though. We need the data to be correct, which this PR does, and we need the UI to be controllable by blocks somehow which we don't have a good API for yet.

users should be prevented from publishing a post or from updating a published post.

Just to put another option out there, rather than preventing, the save button could still be clickable, but the save request could wait for the image upload to be complete before initiating. This way it could be almost imperceptible for users, except for saving taking a bit longer while the two requests complete.

Hey @youknowriad 👋

Would you be able to help write a Dev Note for this feature? We are tracking all the dev notes for 6.7 them in this tracking issue: #65784

We are hoping to have all drafts compiled by October 13th so we have some time to review before RC1

We should include this one with the "stabilize role dev note" but yeah I can write a section for it.

Good call :) Will group them in the tracking issue 👍